Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add base concept of entities to signals #2977

Merged
merged 16 commits into from
Feb 13, 2023
Merged

Conversation

wssheldon
Copy link
Contributor

@wssheldon wssheldon commented Feb 10, 2023

Right now, the only type of Entity implemented will recursively search a signal for a entity that matches a regular expression (as opposed to specify, say the field/key name, where that entity should be). I will end up implementing both as an option. There is also the concept of a "global" entity, where we will search all signals, recursively, for that entity.

We should prioritize "time-to-first-render" and not let entity correlation be a bottleneck, although I expect this will still be fairly performant, all things considered. Only after a signal/case has been fully processed, will we correlate entities with other cases and signals. We can use UI tricks to try to make this invisible to the user and our UI is already set up well for this, by putting information with tabs on the CaseEditSheet.

Case View Mockup

Screenshot 2023-02-10 at 1 13 12 PM

Implemented Entity Case View

case_entitiy_tab

You can click the red context cards and view the signals. Cases table dialog not implemented yet.

@wssheldon wssheldon added the enhancement New feature or request label Feb 10, 2023
db_session.commit()


def get_cases_with_entity(db: Session, entity_id: int, days_back: int) -> int:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def get_cases_with_entity(db: Session, entity_id: int, days_back: int) -> int:
def get_cases_with_entity(db: Session, entity_id: int, days_back: int) -> list[Case]:


@router.get("/{entity_id}/signal_instances", response_model=None)
def get_signal_instances_by_entity(entity_id: int, db: Session = Depends(get_db)):
instances = get_signal_instances_with_entity(db, entity_id, days_back=30)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to ship this with the 30 day default for now and then add a datetime select after mvp?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we worried about returning too much data? If so, we should limit the number of rows returned via limit instead of picking some arbitrary date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the days_back was there to add support for a date range picker on the entity tab (not implemented yet). we probably do care about too much data though as well.

@wssheldon wssheldon marked this pull request as ready for review February 13, 2023 20:33
@wssheldon
Copy link
Contributor Author

Global entity types are not implemented. I will remove from front-end, but keep it in the model.

Copy link
Contributor

@kevgliss kevgliss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, some small suggestions

src/dispatch/entity/models.py Outdated Show resolved Hide resolved
src/dispatch/entity/models.py Outdated Show resolved Hide resolved
# If a field was specified for this entity type, only search that field
if not field or key == field:
# Search the string for matches to the entity type's regular expression
if match := entity_regex.search(val):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we define a field but not a regex? Shouldn't we just return whatever is in the value of that field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now setting a regex is required. That's a good point though if we just want the whole value and know it's always going to be in a certain field in a certain signal. I guess right now you'd just set the field and the regex to .* or a pattern for the value. We should probably support no regex though.

src/dispatch/entity/service.py Outdated Show resolved Hide resolved
src/dispatch/entity/views.py Outdated Show resolved Hide resolved

@router.get("/{entity_id}/signal_instances", response_model=None)
def get_signal_instances_by_entity(entity_id: int, db: Session = Depends(get_db)):
instances = get_signal_instances_with_entity(db, entity_id, days_back=30)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we worried about returning too much data? If so, we should limit the number of rows returned via limit instead of picking some arbitrary date.

src/dispatch/static/dispatch/src/entity/EntityCard.vue Outdated Show resolved Hide resolved
this.isLoading = false;
},
methods: {
async getSignalInstances() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you have to make these async? It's not something I've seen before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted the code above to be async and didn't have another non-async version of the getSignal... api call. We could add a non-async version probably.

src/dispatch/static/dispatch/src/entity/api.js Outdated Show resolved Hide resolved
src/dispatch/static/dispatch/src/entity_type/api.js Outdated Show resolved Hide resolved
@wssheldon
Copy link
Contributor Author

lgtm, some small suggestions

thanks for looking at it

@wssheldon wssheldon merged commit 867679f into master Feb 13, 2023
@wssheldon wssheldon deleted the enhancement/entities branch February 13, 2023 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants